Fix wrong warnings about bad flags assigned to good qualities #2583
Conversation
|
Hello @singiamtel , @ktf , it seems that our builds are all broken: o2 and o2-dataflow-slc9 do not report any status, while macOS-arm fails: could you please have a look? |
|
Hi @knopers8, we're investigating |
|
Thanks! |
Due to the order of processing in WorstOfAllAggregator, we could get cases when a bad Flag (coming from Bad quality) would be assigned to Good quality, while the quality itself would be replaced just after. This commit reverses the order, which will silence the wrong warning. The same code was also reused TrendCheck, thus I am fixing it also there.
knopers8
force-pushed
the
fix-bad-flag-warning
branch
from
aa8b935 to
1d85d6b
Compare
|
@knopers8 the issue should be fixed now. Thanks for the report |
| } | ||
| for (const auto& flag : quality.getFlags()) { | ||
| result.addFlag(flag.first, flag.second); | ||
| } |
There was a problem hiding this comment.
I am sorry but it isn't obvious for me how just reversing order of these two operations in one loop would fix the issue. you are creating Quality object, setting it's quality to the one you find as worse (finding the worst) ... aka setting it's level and name... and than adding all flags from all of the qualities encountered in check. So unless I am missing something I don't understand how you achieve with different situation than before.
There was a problem hiding this comment.
nevermind, I understand, there are internal checks in addFlag ... but just reading the PR isn't obvious why this should fix anything. I would expect that addFlag is just adding flag without any internal checks.
There was a problem hiding this comment.
I think it's quite normal that functions validate their inputs, but indeed I could have mentioned where the warning was coming from.
3 participants
Due to the order of processing in WorstOfAllAggregator, we could get cases when a bad Flag (coming from Bad quality) would be assigned to Good quality, while the quality itself would be replaced just after. This commit reverses the order, which will silence the wrong warning.